Skip to content

refactor: introduce function for getting Schema Object type #10330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

char0n
Copy link
Member

@char0n char0n commented Feb 28, 2025

use function for getting schema object type/schema object type label depending on the OpenAPI version


Attribution: @robert-hebel-sb


export const getSchemaObjectTypeLabel = (label) => label

export const getSchemaObjectType = (schema) => schema.type ?? "any"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with JSON Schema 2020-12 processing, can we make the fallback type the string type?

Suggested change
export const getSchemaObjectType = (schema) => schema.type ?? "any"
export const getSchemaObjectType = (schema) => schema.type ?? "string"

Also, may I suggest to make this function safe:

Suggested change
export const getSchemaObjectType = (schema) => schema.type ?? "any"
export const getSchemaObjectType = (schema) => schema?.type ?? "string"

Copy link
Member Author

@char0n char0n Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferred alternative: What if both the functions would have the following signature?

getSchemaObjectType: (schema: object) -> string
getSchemaObjectTypeLabel: (schema: object) -> string

This would simplify their usage inside the code as it is now abstracted how the the type or label are computed from the schema.

Instead of

const typeLabel = fn.getSchemaObjectTypeLabel(immutableToJS(schema?.get("type")))

you'll end up having

const typeLabel = fn.getSchemaObjectTypeLabel(immutableToJS(schema))

If you further abstract and allow the functions to consume immutable along with POJO, it can look like:

const typeLabel = fn.getSchemaObjectTypeLabel(schema)

NOTE: this applies both for Draft 5 and 2020-12 functions.

@@ -635,3 +635,7 @@ const resolver = (arg1, arg2, arg3) => [arg1, JSON.stringify(arg2), JSON.stringi
export const memoizedCreateXMLExample = memoizeN(createXMLExample, resolver)

export const memoizedSampleFromSchema = memoizeN(sampleFromSchema, resolver)

export const getSchemaObjectTypeLabel = (label) => label
Copy link
Member Author

@char0n char0n Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input is type not a label.

Suggested change
export const getSchemaObjectTypeLabel = (label) => label
export const getSchemaObjectTypeLabel = (type) => type

But anyway we can make this point-free by:

import identity from "lodash/identity"

export const getSchemaObjectTypeLabel = identity

const typeLabel = fn.jsonSchema202012.getType(immutableToJS(schema))
const type = fn.jsonSchema202012.foldType(immutableToJS(schema?.get("type")))
const itemType = fn.jsonSchema202012.foldType(immutableToJS(schema?.getIn(["items", "type"])))
const typeLabel = fn.getSchemaObjectType(immutableToJS(schema))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit confusing. For getting a typeLabel we're using getSchemaObjectType. For actual type we're using getSchemaObjectTypeLabel. Shouldn't it be reversed?

@robert-hebel-sb robert-hebel-sb force-pushed the feat/schema-object-type-func branch from 536f636 to c70aaaf Compare May 5, 2025 10:14
@@ -154,3 +155,11 @@ export const inferType = (schema, processedSchemas = new WeakSet()) => {
export const getType = (schema) => {
return inferType(schema)
}

export const getFoldType = (schema) => {
if (isImmutable(schema)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immutableToJS has the isImmutable check, so we should be able to skip checking it here.

Also, should we have a separate variable for the transformed schema, instead of reassigning?

Same for other places where this occurs.

@robert-hebel-sb robert-hebel-sb requested a review from glowcloud May 14, 2025 11:36
@robert-hebel-sb robert-hebel-sb requested a review from glowcloud May 16, 2025 09:44
Copy link
Contributor

@glowcloud glowcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also revert the changes to unrelated files, e.g. operation-tag? If not possible, then we can leave them as is.

@@ -322,7 +321,7 @@ export default class ParameterRow extends Component {
{ !required ? null : <span>&nbsp;*</span> }
</div>
<div className="parameter__type">
{ typeLabel }
{ schemaObjectType }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using schemaObjectTypeLabel here

Suggested change
{ schemaObjectType }
const schemaObjectTypeLabel = fn.getSchemaObjectTypeLabel(schema)
// ...
{ schemaObjectTypeLabel }

@@ -33,6 +33,8 @@ function afterLoad({ fn, getSystem }) {
getXmlSampleSchema: fn.jsonSchema202012.getXmlSampleSchema,
getSampleSchema: fn.jsonSchema202012.getSampleSchema,
mergeJsonSchema: fn.jsonSchema202012.mergeJsonSchema,
getSchemaObjectTypeLabel: fn.jsonSchema202012.getType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getSchemaObjectTypeLabel: fn.jsonSchema202012.getType,
getSchemaObjectTypeLabel: (schema) => fn.jsonSchema202012.getType(immutableToJS(schema)),

@@ -33,6 +33,8 @@ function afterLoad({ fn, getSystem }) {
getXmlSampleSchema: fn.jsonSchema202012.getXmlSampleSchema,
getSampleSchema: fn.jsonSchema202012.getSampleSchema,
mergeJsonSchema: fn.jsonSchema202012.mergeJsonSchema,
getSchemaObjectTypeLabel: fn.jsonSchema202012.getType,
getSchemaObjectType: fn.jsonSchema202012.foldType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getSchemaObjectType: fn.jsonSchema202012.foldType,
getSchemaObjectType: (schema) => fn.jsonSchema202012.foldType(immutableToJS(schema)?.type),

@@ -42,7 +44,7 @@ const JSONSchema202012SamplesPlugin = ({ getSystem }) => {
getXmlSampleSchema,
getSampleSchema,
mergeJsonSchema,
foldType,
foldType: makeFoldType,
Copy link
Contributor

@glowcloud glowcloud May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the signature of foldType the same when exposed through the plugin system and instead handle immutable in oas31/after-load: #10330 (comment)

@@ -164,7 +165,7 @@ export const makeGetType = (fnAccessor) => {
return combinedStrings || "any"
}

return getType
return (schema, ...rest) => getType(immutableToJS(schema), ...rest)
Copy link
Contributor

@glowcloud glowcloud May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's handle this in oas31/after-load: #10330 (comment)

Suggested change
return (schema, ...rest) => getType(immutableToJS(schema), ...rest)
return getType

@@ -627,6 +628,37 @@ export const createXMLExample = (schema, config, o) => {
return XML(json, { declaration: true, indent: "\t" })
}

const getType = (schema, processedSchemas = new WeakSet()) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is related to rendering, let's move getType and getSchemaObjectTypeLabel to fn in json-schema-5 plugin.

if (schemaItemsType && schemaItemsFormat) {
ArrayItemsComponent = getComponent(`JsonSchema_${schemaItemsType}_${schemaItemsFormat}`)
} else if (schemaItemsType === "boolean" || schemaItemsType === "array" || schemaItemsType === "object") {
ArrayItemsComponent = getComponent(`JsonSchema_${schemaItemsType}`)
}

if (List.isList(schemaItemsType) && (foldedSchemaItemsType === "array" || foldedSchemaItemsType === "object")) {
if (List.isList(schemaItems.get("type")) && (schemaItemsType === "array" || schemaItemsType === "object")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to do this to be safe here

Suggested change
if (List.isList(schemaItems.get("type")) && (schemaItemsType === "array" || schemaItemsType === "object")) {
if (List.isList(schemaItems?.get("type")) && (schemaItemsType === "array" || schemaItemsType === "object")) {


export const getSchemaObjectTypeLabel = (schema) => getType(immutableToJS(schema))

export const getSchemaObjectType = (schema) => schema?.get("type") ?? "string"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about it but maybe we should we be handling immutable here as well?

Suggested change
export const getSchemaObjectType = (schema) => schema?.get("type") ?? "string"
export const getSchemaObjectType = (schema) => immutableToJS(schema)?.type ?? "string"

import memoizeN from "core/utils/memoizeN"
import { immutableToJS } from "../../../utils"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { immutableToJS } from "../../../utils"
import { immutableToJS } from "core/utils"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants